-
Notifications
You must be signed in to change notification settings - Fork 12.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[AIX] Fix XCOFF metadata #118905
[AIX] Fix XCOFF metadata #118905
Conversation
(rustbot has picked a reviewer for you, use r? to override) |
BTW, we are seeking feedback on adding buildbot for AIX. See https://users.rust-lang.org/t/rfc-add-ci-builder-for-aix/103809/2. If we have CI for AIX, such errors can be avoided in the first place. |
cc @saethlin |
I believe the decision on whether and how to add AIX to CI is a matter for @rust-lang/infra. |
@@ -158,12 +158,13 @@ pub(super) fn get_metadata_xcoff<'a>(path: &Path, data: &'a [u8]) -> Result<&'a | |||
file.symbols().find(|sym| sym.name() == Ok(AIX_METADATA_SYMBOL_NAME)) | |||
{ | |||
let offset = metadata_symbol.address() as usize; | |||
if offset < 8 { | |||
// The offset specifies the location of rustc metadata in the .info section of XCOFF. | |||
// Each string stored in .info section of XCOFF is preceded by a 4-byte lenght field. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Each string stored in .info section of XCOFF is preceded by a 4-byte lenght field. | |
// Each string stored in .info section of XCOFF is preceded by a 4-byte length field. |
@@ -599,12 +600,12 @@ pub fn create_compressed_metadata_file_for_xcoff( | |||
section: SymbolSection::Section(data_section), | |||
flags: SymbolFlags::None, | |||
}); | |||
let len = data.len() as u64; | |||
let offset = file.append_section_data(section, &len.to_le_bytes(), 1); | |||
let len = data.len() as u32; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The length of data
here is not guaranteed to fit in a u32
. Ideally we'd produce a compile error here, but it's not clear to me if we have access to a TyCtxt
to do that. At the very least we should use .try_into().unwrap()
to ICE early instead of producing a truncated metadata section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the very least there is a session:
return create_compressed_metadata_file_for_xcoff(file, &packed_metadata, symbol_name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bzEq even though we panic above, this should also be .try_into().unwrap()
@rustbot ready |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r=me with the nitpick
@bors delegate+
@@ -599,12 +600,12 @@ pub fn create_compressed_metadata_file_for_xcoff( | |||
section: SymbolSection::Section(data_section), | |||
flags: SymbolFlags::None, | |||
}); | |||
let len = data.len() as u64; | |||
let offset = file.append_section_data(section, &len.to_le_bytes(), 1); | |||
let len = data.len() as u32; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bzEq even though we panic above, this should also be .try_into().unwrap()
I said @bors delegate+ |
✌️ @bzEq, you can now approve this pull request! If @WaffleLapkin told you to " |
…iaskrgr Rollup of 5 pull requests Successful merges: - rust-lang#118852 (coverage: Skip instrumenting a function if no spans were extracted from MIR) - rust-lang#118905 ([AIX] Fix XCOFF metadata) - rust-lang#118967 (Add better ICE messages for some undescriptive panics) - rust-lang#119051 (Replace `FileAllocationInfo` with `FileEndOfFileInfo`) - rust-lang#119059 (Deny `~const` trait bounds in inherent impl headers) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#118905 - bzEq:revert-u64-on-xcoff, r=WaffleLapkin [AIX] Fix XCOFF metadata rust-lang#118344 accidentally changed the way to get metadata from XCOFF file and broken our internal CI. This PR reverts part of rust-lang#118344 .
#118344 accidentally changed the way to get metadata from XCOFF file and broken our internal CI.
This PR reverts part of #118344 .